-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: edge-to-edge mode on Android #52392
Conversation
@shawnborton would it be possible to ask you to test this PR? This PR should unify how Android app is handling insets (now it'll be identical to iOS). This PR is a logical continuation of this #51956 but the key difference is that now we don't manage color of navigation bar independtly - now we make all system bars (navigation bar, status bar) transparent by default, so it matches iOS app style. The reason why I want you to test and give your feedback, is because some elements look good on iOS, but looks not very good on Android. For example, navigation bar padding on Android vs iOS:
This is the one issue I spotted at the moment, but maybe you find more - your feedback is very valuable 🙏 @mountiny would it be possible to trigger a build so that @shawnborton can test the app? |
I can trigger a test build. I don't have an Android with me at the moment but I believe @dubielzyk-expensify can help us test! |
Sounds good! Any look from UX/UI team will help a lot ❤️ |
We might need to take the PR out of draft in order to run the test build. If we don't get a test build posted in the next 20 minutes or so, let's try that. |
This comment has been minimized.
This comment has been minimized.
Well wouldya just look at that, ready for testing. |
Assigned @c3024 for c+ review |
Nice testing Jon, thanks for running through the screens and I agree with your feedback. |
Awesome @dubielzyk-expensify ! 🔥 Thank you for your feedback! |
Do we need a new build @kirillzyusko |
@mountiny no, not yet. Tomorrow I want to test on more devices and if everything works well, then I will ask for additional testing 😊 |
package.json
Outdated
@@ -113,6 +113,7 @@ | |||
"expo-av": "14.0.7", | |||
"expo-image": "1.12.15", | |||
"expo-image-manipulator": "12.0.5", | |||
"expo-navigation-bar": "~3.0.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add this library again, because we need to manage navigation bar button style (dark/light).
Since user can customize the theme of app independently to device settings we also have to manage it dynamically from JS code.
I checked and one method that we use (setButtonStyleAsync
) doesn't use deprecated API. However I remember, that this particular library wasn't accepted in the first round of fixing the nav bar color management, so i'm happy to discuss further steps here 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirillzyusko I noticed you are adding the library here again even though in Slack we agreed not to use it https://expensify.slack.com/archives/C01GTK53T8Q/p1731618817760309?thread_ts=1731092817.909359&cid=C01GTK53T8Q
Can you please come back to the thread and summarize the problems you are facing with not using the library or explain clearly why we cannot just handle this on our own? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Vit, I missed this but agree with needing to discuss this further.
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comments
android/app/src/main/java/com/expensify/chat/navbar/NavBarManagerModule.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/expensify/chat/navbar/NavBarManagerPackage.kt
Outdated
Show resolved
Hide resolved
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@c3024 let us know when you complete the testing please! thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android: Native
WhatsApp.Video.2024-11-25.at.20.52.49.mp4
WhatsApp.Video.2024-11-25.at.21.28.15.mp4
Android: mWeb Chrome
WhatsApp.Video.2024-11-25.at.20.57.38.mp4
iOS: Native
RPReplay_Final1732548627.MP4
iOS: mWeb Safari
RPReplay_Final1732548716.MP4
MacOS: Chrome / Safari
endChrome.mov
MacOS: Desktop
Screen.Recording.2024-11-25.at.9.23.19.PM.mov
Tests well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Mobile-Expensify pr merged |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.67-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.67-9 🚀
|
Explanation of Change
KeyboardAvoidingView
for Android - ideally we should use the implementation fromreact-native-keyboard-controller
, but in this case animations have a very poor performance and it's better to investigate/fix as a separate issueModal
(that addsnavigationBarTranslucent
property - applied forreact-native
andreact-native-modal
)Linked HybridApp PR: https://github.com/Expensify/Mobile-Expensify/pull/13299
Fixed Issues
$ #51673
$ #52116
PROPOSAL: #52116 (comment)
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop